Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix redoPlacement race condition #5185

Merged
merged 1 commit into from
Aug 28, 2017
Merged

Fix redoPlacement race condition #5185

merged 1 commit into from
Aug 28, 2017

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 23, 2017

Closes #3700. The condition occurred when a tile requested redoPlacement (which sent a redoPlacement request to the worker) and then shortly got removed (before getting response to redoPlacement back from the worker) — stopPlacementThrottler did not prevent redoPlacement from running in this case because it was past the throttling phase.

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page

@mourner mourner requested a review from ChrisLoer August 23, 2017 14:29
@mourner mourner force-pushed the fix-redo-placement-error branch from d1b1bb5 to a4b7243 Compare August 23, 2017 14:55
@mourner mourner mentioned this pull request Aug 23, 2017
Closed
3 tasks
@mourner
Copy link
Member Author

mourner commented Aug 23, 2017

Not sure if it's worth adding a test for this tricky condition if the placement code is fully rewritten in the viewport-collision branch anyway.

@ChrisLoer
Copy link
Contributor

I think this change would turn the redoWhenDone functionality into a no-op. Looking back even before the throttling changes went in, the behavior used to be that the tile state would be 'loaded', but another placement request would happen. I think that was a bug -- the state should have always been switched to 'reloading'.

Did the throttling changes cause #3700, or did they just change the timing so that it was easier to expose this condition?

@mourner mourner force-pushed the fix-redo-placement-error branch from a4b7243 to 80d3d89 Compare August 28, 2017 08:46
@mourner
Copy link
Member Author

mourner commented Aug 28, 2017

@ChrisLoer fixed it to add the reloading state on redoWhenDone. If you think we'll do one more release before the viewport labeling PR is finalized, this is worth merging, otherwise we should close to reduce merge collisions. Not sure what caused the regression in the first place but throttling could be related.

@mourner mourner mentioned this pull request Aug 28, 2017
Closed
ChrisLoer
ChrisLoer approved these changes Aug 28, 2017
View reviewed changes
Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I think it's worth merging.

@mourner mourner merged commit f8a0b0f into master Aug 28, 2017
@mourner mourner deleted the fix-redo-placement-error branch August 28, 2017 15:41
@jmunowitch jmunowitch mentioned this pull request Sep 14, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisLoer ChrisLoer ChrisLoer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mourner @ChrisLoer